-
Notifications
You must be signed in to change notification settings - Fork 724
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
CAD-2337 API extensions for address and credentials #2127
Conversation
cardano-api/src/Cardano/API.hs
Outdated
fromShelleyPaymentCredential, | ||
fromShelleyStakeCredential, | ||
StakeAddressReference(..), | ||
fromShelleyStakeReference, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
w.r.t. these exports you've added, if you need them, could we export them from Cardano.Api.Shelley
instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, these should only be exported from Cardano.Api.Shelley
and not from Cardano.Api
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, fine to add these definitions and export them from appropriate modules.
Comments below. Please follow the local style.
toAddressAny = \case | ||
a@ShelleyAddress{} -> AddressShelley a | ||
a@ByronAddress{} -> AddressByron a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
local style would be
toAddressAny a@ShelleyAddress{} = AddressShelley a
toAddressAny a@ByronAddress{} = AddressByron a
no need for case or lambda case at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to be exported, both from this module and from Cardano.API
and Cardano.Api.Typed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
@@ -1,5 +1,6 @@ | |||
{-# LANGUAGE FlexibleInstances #-} | |||
{-# LANGUAGE GADTs #-} | |||
{-# LANGUAGE LambdaCase #-} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't the local style.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
@@ -32,6 +33,7 @@ module Cardano.Api.Address ( | |||
shelleyAddressInEra, | |||
anyAddressInShelleyBasedEra, | |||
anyAddressInEra, | |||
toAddressAny, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Put this in the previous patch, where it gets defined.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
@@ -51,6 +53,10 @@ module Cardano.Api.Address ( | |||
fromShelleyStakeAddr, | |||
fromShelleyStakeCredential, | |||
|
|||
fromShelleyPaymentCredential, | |||
fromShelleyStakeCredential, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Huh? This is already exported.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, thank you, missed it.
fromShelleyPaymentCredential :: | ||
Shelley.PaymentCredential StandardShelley -> PaymentCredential | ||
fromShelleyPaymentCredential = \case | ||
Shelley.KeyHashObj kh -> PaymentCredentialByKey (PaymentKeyHash kh) | ||
Shelley.ScriptHashObj sh -> PaymentCredentialByScript (ScriptHash sh) | ||
|
||
fromShelleyStakeReference :: | ||
Shelley.StakeReference StandardShelley -> StakeAddressReference | ||
fromShelleyStakeReference = \case | ||
Shelley.StakeRefBase stakecred -> StakeAddressByValue (fromShelleyStakeCredential stakecred) | ||
Shelley.StakeRefPtr ptr -> StakeAddressByPointer ptr | ||
Shelley.StakeRefNull -> NoStakeAddress |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we follow the local style (e.g. the functions immediately above) and use
foo (Shelley.Blah ..) = Blah ...
rather than lambda case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
cardano-api/src/Cardano/API.hs
Outdated
fromShelleyPaymentCredential, | ||
fromShelleyStakeCredential, | ||
StakeAddressReference(..), | ||
fromShelleyStakeReference, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, these should only be exported from Cardano.Api.Shelley
and not from Cardano.Api
b7e7704
to
8661c8c
Compare
All suggestions addressed. |
8661c8c
to
7af20aa
Compare
7af20aa
to
8c1afb3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
bors r+ |
2125: Update cardano-node-cli-reference.md r=kevinhammond a=kevinhammond Added additional documentation for query ledger/protocol-state 2127: CAD-2337 API extensions for address and credentials r=deepfire a=deepfire - `toAddressAny :: Address addr -> AddressAny` - `fromShelleyPaymentCredential :: Shelley.PaymentCredential StandardShelley -> PaymentCredential` - `fromShelleyStakeReference :: Shelley.StakeReference StandardShelley -> StakeAddressReference` - export `fromShelleyStakeCredential` 2130: Testnet with more configurable parameters r=newhoggy a=newhoggy Co-authored-by: Kevin Hammond <12563287+kevinhammond@users.noreply.github.com> Co-authored-by: Kosyrev Serge <serge.kosyrev@iohk.io> Co-authored-by: John Ky <john.ky@iohk.io>
This PR was included in a batch that timed out, it will be automatically retried |
Build succeeded: |
toAddressAny :: Address addr -> AddressAny
fromShelleyPaymentCredential :: Shelley.PaymentCredential StandardShelley -> PaymentCredential
fromShelleyStakeReference :: Shelley.StakeReference StandardShelley -> StakeAddressReference
fromShelleyStakeCredential